Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bluetooth: Controller: Ensure controller can handle flow control buffers #19869

Merged
merged 1 commit into from
Feb 18, 2025
Merged

Bluetooth: Controller: Ensure controller can handle flow control buffers #19869

merged 1 commit into from
Feb 18, 2025

Conversation

KyraLengfeld
Copy link
Contributor

@KyraLengfeld KyraLengfeld commented Jan 13, 2025 •
edited
Loading

The host will generate up to acl_pkts number of Host Number of Completed
Packets plus a number of normal HCI commands, which use
Num_HCI_Command_Packets.
As such, if we do not provide enough (host) TX CMD buffers for the
possible number of acl_pkts plus Num_HCI_Command_Packets, we may run
into a deadlock.

Currently the SDC controller delays sending disconnect events until it
received all the ACKs for the ACL data packets sent to the host to avoid
race conditions.
Which, in case there aren't enough (host) TX CMD buffers, can result in
a deadlock visible by a missing disconnect event.

When Controller to Host data flow control is supported, this commit
ensures that the CONFIG_BT_BUF_CMD_TX_COUNT is greater than or equal
to BT_BUF_ACL_RX_COUNT + Ncmd, where Ncmd is the supported maximum
Num_HCI_Command_Packets.
Note: The SDC controller (currently) does not support
Num_HCI_Command_Packets > 1. Ncmd is always 1.

Furthermore this commit restricts the host_total_num_acl_data_packets
communicated to the controller to how many acknowledgements + Ncmd the
controller is able to receive from the host. This is especially valuable
in a controller-only build where it cannot be foreseen what settings are
used for the host.

@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jan 13, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 13, 2025 •
edited
Loading

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 25

Inputs:

Sources:

sdk-nrf: PR head: 6c02a2b307297638bb92c18ccea514ea4326e71b

more details

sdk-nrf:

PR head: 6c02a2b307297638bb92c18ccea514ea4326e71b
merge base: f79b82934dfb0b444d930aaca134fff088e4ce77
target head (main): 9185c68a324540178aa05ae9d60ee59098d6a059
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (12) 
CODEOWNERS
boards
│  ├── nordic
│  │  ├── thingy91
│  │  │  │ thingy91_nrf52840_defconfig
doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  │ known_issues.rst
scripts
│  ├── ci
│  │  │ tags.yaml
subsys
│  ├── bluetooth
│  │  ├── controller
│  │  │  ├── CMakeLists.txt
│  │  │  ├── hci_internal.c
│  │  │  ├── hci_internal_wrappers.c
│  │  │  │ hci_internal_wrappers.h
tests
│  ├── subsys
│  │  ├── bluetooth
│  │  │  ├── controller
│  │  │  │  ├── CMakeLists.txt
│  │  │  │  ├── prj.conf
│  │  │  │  ├── src
│  │  │  │  │  │ hci_cmd_cb_host_buffer_size_test.c
│  │  │  │  │ testcase.yaml

Outputs:

Toolchain

Version: 0f22b642ed
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:0f22b642ed_bbe5b33786

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 24
  • ✅ Integration tests
    • ✅ desktop52_verification - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test_ble_nrf_config
    • ✅ test-fw-nrfconnect-ble_samples - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-chip - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-zigbee - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-find-my - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-sidewalk - Skipped: Job was skipped as it succeeded in a previous run
Disabled integration tests
    • doc-internal
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-proprietary_esb
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

@KyraLengfeld KyraLengfeld force-pushed the cmd_buffer_workaround branch from 7e4cf76 to 7dd9904 Compare January 13, 2025 12:37
@KyraLengfeld KyraLengfeld marked this pull request as ready for review January 13, 2025 12:37
@KyraLengfeld KyraLengfeld requested review from a team as code owners January 13, 2025 12:37
@KyraLengfeld KyraLengfeld requested a review from cvinayak January 13, 2025 12:37
peknis
peknis requested changes Jan 13, 2025
View reviewed changes
@KyraLengfeld KyraLengfeld force-pushed the cmd_buffer_workaround branch 2 times, most recently from e9664c2 to 5fabd4a Compare January 13, 2025 16:25
peknis
peknis requested changes Jan 14, 2025
View reviewed changes
@KyraLengfeld KyraLengfeld force-pushed the cmd_buffer_workaround branch from 5fabd4a to b1c56cf Compare January 14, 2025 08:53
@KyraLengfeld KyraLengfeld force-pushed the cmd_buffer_workaround branch 2 times, most recently from 0459e3f to c1ceef7 Compare January 17, 2025 15:09
cvinayak
cvinayak previously requested changes Jan 20, 2025
View reviewed changes
@KyraLengfeld KyraLengfeld force-pushed the cmd_buffer_workaround branch from c1ceef7 to eba935e Compare January 21, 2025 16:28
@KyraLengfeld KyraLengfeld requested a review from a team as a code owner February 10, 2025 12:26
seko-nordic
seko-nordic previously requested changes Feb 10, 2025
View reviewed changes
@KyraLengfeld KyraLengfeld force-pushed the cmd_buffer_workaround branch 3 times, most recently from 785a9ab to d52ce4c Compare February 11, 2025 08:44
@seko-nordic seko-nordic dismissed their stale review February 11, 2025 08:57

All my comments are addressed. I would prefer if someone familiar with Zephyr host approved this PR.

@nrfconnect nrfconnect deleted a comment from divipillai Feb 13, 2025
@nrfconnect nrfconnect deleted a comment from divipillai Feb 13, 2025
@nrfconnect nrfconnect deleted a comment from divipillai Feb 13, 2025
@KyraLengfeld KyraLengfeld dismissed katgiadla’s stale review February 17, 2025 13:46

Changed according to her change request (one-liner) and she didn't respond to my re-review request.

@KyraLengfeld KyraLengfeld force-pushed the cmd_buffer_workaround branch 2 times, most recently from 753b59d to 96748e3 Compare February 18, 2025 07:43
@KyraLengfeld
Bluetooth: Controller: Ensure controller can handle flow control buffers
6c02a2b 
The host will generate up to acl_pkts number of Host Number of Completed
Packets plus a number of normal HCI commands, which use
Num_HCI_Command_Packets.
As such, if we do not provide enough (host) TX CMD buffers for the
possible number of acl_pkts plus Num_HCI_Command_Packets, we may run
into a deadlock.

Currently the SDC controller delays sending disconnect events until it
received all the ACKs for the ACL data packets sent to the host to avoid
race conditions.
Which, in case there aren't enough (host) TX CMD buffers, can result in
a deadlock visible by a missing disconnect event.

When Controller to Host data flow control is supported, this commit
ensures that the `CONFIG_BT_BUF_CMD_TX_COUNT` is greater than or equal
to `BT_BUF_ACL_RX_COUNT + Ncmd`, where Ncmd is the supported maximum
Num_HCI_Command_Packets.
Note: The SDC controller (currently) does not support
Num_HCI_Command_Packets > 1. Ncmd is always 1.

Furthermore this commit restricts the `host_total_num_acl_data_packets`
communicated to the controller to how many acknowledgements + Ncmd the
controller is able to receive from the host. This is especially valuable
in a controller-only build where it cannot be foreseen what settings are
used for the host.

Signed-off-by: Kyra Lengfeld <[email protected]>
@nordicjm nordicjm merged commit 7766183 into nrfconnect:main Feb 18, 2025
13 checks passed
@KyraLengfeld KyraLengfeld deleted the cmd_buffer_workaround branch February 18, 2025 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtguggedal jtguggedal jtguggedal left review comments

@rugeGerritsen rugeGerritsen rugeGerritsen left review comments

@cvinayak cvinayak cvinayak approved these changes

@nordicjm nordicjm nordicjm approved these changes

@alxelax alxelax alxelax approved these changes

@alwa-nordic alwa-nordic alwa-nordic approved these changes

@peknis peknis peknis approved these changes

@divipillai divipillai divipillai approved these changes

@richabp richabp richabp approved these changes

@nordic-piks nordic-piks nordic-piks approved these changes

@shanthanordic shanthanordic shanthanordic approved these changes

@seko-nordic seko-nordic seko-nordic left review comments

@PerMac PerMac Awaiting requested review from PerMac PerMac is a code owner

@nordic-auko nordic-auko Awaiting requested review from nordic-auko

@katgiadla katgiadla Awaiting requested review from katgiadla katgiadla is a code owner

Assignees
No one assigned
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.